Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new initial fields to v20240812preview #3478

Merged
merged 10 commits into from
Apr 8, 2024
Merged

Conversation

cadenmarchese
Copy link
Collaborator

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-4381 && https://issues.redhat.com/browse/ARO-4377, unblocks https://issues.redhat.com/browse/ARO-3979

What this PR does / why we need it:

This is the first of 3-ish PRs to add new fields to the preview API + converter. There will be additional fields added to support OCP upgrades as well as Cluster MSI.

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

How do you know this will function as expected in production?

@cadenmarchese
Copy link
Collaborator Author

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested some small changes to make sure we're lining up with what the team agreed on in the design doc.

pkg/api/v20240812preview/openshiftcluster.go Outdated Show resolved Hide resolved
pkg/api/v20240812preview/openshiftcluster.go Outdated Show resolved Hide resolved
pkg/api/openshiftcluster.go Outdated Show resolved Hide resolved
pkg/api/openshiftcluster.go Outdated Show resolved Hide resolved
@kimorris27
Copy link
Contributor

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that we're missing the admin API changes. Let's add those into this PR.

kimorris27
kimorris27 previously approved these changes Mar 29, 2024
@kimorris27
Copy link
Contributor

/azp run ci, e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

tsatam
tsatam previously approved these changes Apr 1, 2024
Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've left one question about handling the existing ServicePrincipalProfile struct on WIMI clusters for my own curiosity, but it shouldn't block this PR and it would likely need to be the subject of its own dedicated PR if we want to tackle it.

pkg/api/admin/openshiftcluster.go Show resolved Hide resolved
s-amann
s-amann previously approved these changes Apr 2, 2024
Copy link
Contributor

@s-amann s-amann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Some minor comments below which could be in a new PR

pkg/api/admin/openshiftcluster.go Show resolved Hide resolved
pkg/api/openshiftcluster.go Show resolved Hide resolved
pkg/api/openshiftcluster.go Show resolved Hide resolved
pkg/api/v20240812preview/openshiftcluster.go Show resolved Hide resolved
@cadenmarchese cadenmarchese dismissed stale reviews from s-amann, tsatam, and kimorris27 via 0016940 April 3, 2024 19:34
@cadenmarchese
Copy link
Collaborator Author

@s-amann should be good now, thanks for pointing that out!

Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked out the CI failure, and it looks like the full results of make client haven't yet been pushed since the most recent changes.

@cadenmarchese
Copy link
Collaborator Author

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@cadenmarchese cadenmarchese merged commit adc4836 into master Apr 8, 2024
19 of 20 checks passed
@mociarain mociarain deleted the new-api-fields branch July 16, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants